Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for slab buffer retention, leading to large memory consumption #1143

Closed
wants to merge 1 commit into from

Conversation

jmatthewsr-ms
Copy link

  • Admittedly undertested. This issues also affects early revs of socket.io. The 'ws' library also exhibits this behavior and a pull request has been created for that lib.

See: https://github.com/jmatthewsr-ms/node-slab-memory-issues

@jmatthewsr-ms
Copy link
Author

Putting this on hold as I believe that a node core fix is the most appropriate. Buffer can also retain an 8k slab on it's own, ideally node core should emit a buffer that is not backed by any slab.

@rauchg
Copy link
Contributor

rauchg commented Jan 26, 2013

Agreed

@3rd-Eden
Copy link
Contributor

@jmatthewsr-ms is there a bug in the joyent/node that we can follow about this?

@3rd-Eden
Copy link
Contributor

Found it: nodejs/node-v0.x-archive#4660

Thanks for the awesome research btw, this is definitely a bit issue we are facing in socket.io and probably engine.io as well.

@3rd-Eden
Copy link
Contributor

As this is not being addressed by the Node core team, it seems to me that we need to address this our selfs. What do you think @guille?

@jpallen
Copy link

jpallen commented Feb 27, 2013

I've just run into this issue myself in production and opened up another issue and pull request (#1177 #1178) before finding this issue. If my vote counts for anything, I like the solution here better than my own. It would be nice if this could make it into 0.9, even if you decide on a better solution for 1.0, since we (and many other people) are using Socket.io in production right now and need this fix.

Edit: You can see from the graphs in #1177 that this is quite a serious memory leak in a production server doing large or many requests over HTTPS.

@jmatthewsr-ms
Copy link
Author

Note that this is being worked on in node core: nodejs/node-v0.x-archive#4964.

The buffer copy in this fix may not be required once node core buffer is fixed. The fix here shouldn't hurt performance if left in, but worth checking once node core is updated.

@timewarrior
Copy link

Hi Everyone, thanks for being on top of this. I had a quick question. I understand that a fix is being worked on node for this and meanwhile japplen's patch has made it into 6e25c80 . We are trying to launch something based on socket.io soon. Can someone please confirm that 6e25c80 fixes the patch being mentioned here.
Thanks

@jpallen
Copy link

jpallen commented Jun 10, 2013

Yes, the patch here has the same effect as 6e25c80. The purpose of both is to dereference the head buffer which is passed to the upgrade event handler since this was the cause of the memory leak.

@timewarrior
Copy link

Thx jpallen :)

@rauchg
Copy link
Contributor

rauchg commented Jul 18, 2014

Fixing on engine.io (socketio/engine.io#268)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants